pin flatc version and regenerate stale code#8
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a pinned flatc compiler versioning system to ensure consistent FlatBuffers code generation across different environments. It adds a script to automatically download the required binary, updates the project's build scripts, and includes updated generated TypeScript files. The review feedback focuses on enhancing the shell script's portability and reliability through better dependency checking and error handling, as well as ensuring a clean state during the FlatBuffers verification process.
scripts/ensure-flatc.sh
Outdated
| @@ -0,0 +1,50 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Using #!/usr/bin/env bash is more portable than #!/bin/bash, as it avoids hardcoding the path to the bash executable which may vary across different Unix-like systems.
| #!/bin/bash | |
| #!/usr/bin/env bash |
| # FLATC="$(./scripts/ensure-flatc.sh)" | ||
| # "$FLATC" -T -o ./src/format/flatbuffers --gen-all ./flatbuffers/all.fbs | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
It's recommended to verify that the required tools (curl and unzip) are available before proceeding with the download and extraction process to provide a clearer error message if they are missing.
| set -euo pipefail | |
| set -euo pipefail | |
| # Check dependencies | |
| for cmd in curl unzip; do | |
| if ! command -v "$cmd" >/dev/null 2>&1; then | |
| echo "error: $cmd is required but not installed." >&2 | |
| exit 1 | |
| fi | |
| done |
scripts/ensure-flatc.sh
Outdated
| trap 'rm -rf "$TMPDIR"' EXIT | ||
|
|
||
| echo "Downloading flatc v${FLATC_VERSION}..." >&2 | ||
| curl -sL "$URL" -o "$TMPDIR/flatc.zip" |
There was a problem hiding this comment.
Adding the -f flag to curl ensures the script fails if the download returns an HTTP error code (like 404). Additionally, consider verifying the checksum of the downloaded binary to ensure its integrity and security.
| curl -sL "$URL" -o "$TMPDIR/flatc.zip" | |
| curl -fsSL "$URL" -o "$TMPDIR/flatc.zip" |
package.json
Outdated
| "generate:fbs": "flatc -T -o ./src/format/flatbuffers --gen-all ./flatbuffers/all.fbs && node scripts/postgen-fbs.cjs ./src/format/flatbuffers && prettier --write src/format/flatbuffers/generated", | ||
| "check:fbs": "flatc -T -o /tmp/fbs-check --gen-all ./flatbuffers/all.fbs && node scripts/postgen-fbs.cjs /tmp/fbs-check && prettier --write /tmp/fbs-check/generated && diff -r /tmp/fbs-check/generated src/format/flatbuffers/generated" | ||
| "generate:fbs": "FLATC=\"$(./scripts/ensure-flatc.sh)\" && \"$FLATC\" -T -o ./src/format/flatbuffers --gen-all ./flatbuffers/all.fbs && node scripts/postgen-fbs.cjs ./src/format/flatbuffers && prettier --write src/format/flatbuffers/generated", | ||
| "check:fbs": "FLATC=\"$(./scripts/ensure-flatc.sh)\" && \"$FLATC\" -T -o /tmp/fbs-check --gen-all ./flatbuffers/all.fbs && node scripts/postgen-fbs.cjs /tmp/fbs-check && prettier --write /tmp/fbs-check/generated && diff -r /tmp/fbs-check/generated src/format/flatbuffers/generated" |
There was a problem hiding this comment.
Cleaning the /tmp/fbs-check directory before running flatc ensures that the comparison is performed against a fresh set of generated files, preventing false positives from stale files left over from previous runs.
| "check:fbs": "FLATC=\"$(./scripts/ensure-flatc.sh)\" && \"$FLATC\" -T -o /tmp/fbs-check --gen-all ./flatbuffers/all.fbs && node scripts/postgen-fbs.cjs /tmp/fbs-check && prettier --write /tmp/fbs-check/generated && diff -r /tmp/fbs-check/generated src/format/flatbuffers/generated" | |
| "check:fbs": "rm -rf /tmp/fbs-check && mkdir -p /tmp/fbs-check && FLATC=\"$(./scripts/ensure-flatc.sh)\" && \"$FLATC\" -T -o /tmp/fbs-check --gen-all ./flatbuffers/all.fbs && node scripts/postgen-fbs.cjs /tmp/fbs-check && prettier --write /tmp/fbs-check/generated && diff -r /tmp/fbs-check/generated src/format/flatbuffers/generated" |
1 participant
Fixes CI error on #7
Two issues fixed:
1. Stale generated code (broke CI): Upstream schema changes (new
node_id/node_typefields onMoveOperation) were pulled in byprepare>sync-flatbuffers.shbut the generated TypeScript was never regenerated, socheck:fbsfailed.2. No pinned
flatcversion (latent risk): CI installed oneflatcversion, local dev could use another. This didn't cause the current failure, but would cause hard-to-debug diffs if versions ever produced different output for the same schema.Fix:
scripts/ensure-flatc.shwhich pins flatc v25.12.19, auto-downloads to.bin/if missinggenerate:fbsandcheck:fbsnpm scripts use the pinned version